roachtest: allow tests to specify a cockroach binary to use #113301

DarrylWong · 2023-10-30T17:39:45Z

Currently, roachtests must manually upload their own cockroach binary if needed through the Put API.
However, almost all roachtests upload the standard t.Cockroach() binary to ./cockroach on all nodes,
resulting in the same Put code being duplicated at the start of most tests. Additionally, to collect artifacts
we still need a cockroach binary at a discoverable path, leading to the same binary being copied twice in many
cases, see: #97814

This change adds a TestSpec option which lets tests specify a cockroach binary to use. If one is not specified,
we now upload the t.Cockroach() binary to ./cockroach. This lets us remove cockroach-default logic for artifacts,
and removes the need to manually upload binaries at the start of each test.

Release note: None
Fixes: #104729

cockroach-teamcity · 2023-10-30T17:39:58Z

This change is

DarrylWong · 2023-10-30T17:40:54Z

Test Run

Looks good, two failures that have been fixed on master and one that still fails on master.

RaduBerinde

This is fantastic, the double copy was very annoying. Nice job!! 👍

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @renatolabs, and @srosenberg)

pkg/cmd/roachtest/cluster.go line 1839 at r1 (raw file):

// runtime assertions enabled. Note that we upload to all nodes even if they
// don't use the binary, so that the test runner can always fetch logs.
func (c *clusterImpl) PutCockroach(ctx context.Context, l *logger.Logger, t *testImpl) error {

[nit] this feels like it belongs with testImpl rather than clusterImpl

pkg/cmd/roachtest/cluster.go line 1848 at r1 (raw file):

		return c.PutE(ctx, l, t.RuntimeAssertionsCockroach(), test.DefaultCockroachPath, c.All())
	default:
		return errors.Errorf("Specified cockroach binary does not exist.")

[nit] error is confusing, sounds like a binary is missing, maybe "unknown CockroachBinary value %v"

pkg/cmd/roachtest/registry/test_spec.go line 434 at r1 (raw file):

// ClusterCockroachBinary is the cockroach binary that will be uploaded
// to every node in the cluster at the start of the test. We upload to
// every node so that we can fetch logs in the case of a failure.

[nit] It's important to mention the default value when it's not specified.

renatolabs · 2023-11-07T18:53:52Z

Very exciting to see all these c.Put calls going away!

Nit: can we make the PR title match the commit?

Thought: should we make t.Cockroach() private? Is there still a legitimate use for it?

The thinking is that muscle memory plays a strong role in roachtest, and it's only a matter of time before this c.Put call is added back to new or existing tests. A compilation failure would be a pretty good indication that that should not be done anymore.

Doesn't have to be part of this PR, just wanted to ask the question.

renatolabs

Reviewed 111 of 111 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DarrylWong, @RaduBerinde, and @srosenberg)

pkg/cmd/roachtest/cluster.go line 1839 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] this feels like it belongs with testImpl rather than clusterImpl

Not sure about that; this function is uploading a file to the cluster. Most other functions that do this live in the cluster level (e.g., PutLibraries, PutString, Put itself, Stage, etc.)

DarrylWong

Argh, just noticed a few t.Cockroach() slipped through because they call c.PutE. Will have to rerun those but since they just upload cockroach to every node I'm confident there shouldn't be any issue with them.

Nit: can we make the PR title match the commit?

Done.

should we make t.Cockroach() private? Is there still a legitimate use for it?

The autoupgrade and schemachange/mixed-versions still use it. The latter should be ported to the new mixed-version framework though so not sure if it will be still using it then. Besides those two, it's not being called anywhere else in roachtests so yes I think we could make it private if autoupgrade is addressed.

I didn't dig too deeply into it, but is there a reason why we can't port autoupgrade to the new framework? I see it has the warning at the top to not use it for mixed-version but it doesn't explain why.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde, @renatolabs, and @srosenberg)

pkg/cmd/roachtest/cluster.go line 1839 at r1 (raw file):

Previously, renatolabs (Renato Costa) wrote…

Not sure about that; this function is uploading a file to the cluster. Most other functions that do this live in the cluster level (e.g., PutLibraries, PutString, Put itself, Stage, etc.)

I'm content with either location but yeah Renato's logic is why I originally had it in clusterImpl.

pkg/cmd/roachtest/registry/test_spec.go line 434 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] It's important to mention the default value when it's not specified.

Done but also moved it along with other parts of the comment to the TestSpec struct to better reflect how the other comments are.

RaduBerinde · 2023-11-07T20:50:53Z

pkg/cmd/roachtest/cluster.go line 1839 at r1 (raw file):

Previously, DarrylWong wrote…

I'm content with either location but yeah Renato's logic is why I originally had it in clusterImpl.

Well.. this function is deciding which binary to upload and then calls Put to upload. Is the cluster or the test responsible for that decision?

More generally - should clusterImpl have to understand anything about testImpl? If it's not necessary, it shouldn't. I know there are a few uses of testImpl already but they are pretty small and easily fixable.

renatolabs

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong, @RaduBerinde, and @srosenberg)

pkg/cmd/roachtest/cluster.go line 1839 at r1 (raw file):

Previously, RaduBerinde wrote…

Well.. this function is deciding which binary to upload and then calls Put to upload. Is the cluster or the test responsible for that decision?

More generally - should clusterImpl have to understand anything about testImpl? If it's not necessary, it shouldn't. I know there are a few uses of testImpl already but they are pretty small and easily fixable.

IMO, there's no decision being made here. The cluster is reading the option chosen by the user, if any, and calling Put based on that. In case the choice is randomized, the actual choosing logic is in the testImpl.

RaduBerinde · 2023-11-07T21:03:49Z

pkg/cmd/roachtest/cluster.go line 1839 at r1 (raw file):

Previously, renatolabs (Renato Costa) wrote…

IMO, there's no decision being made here. The cluster is reading the option chosen by the user, if any, and calling Put based on that. In case the choice is randomized, the actual choosing logic is in the testImpl.

Fine with me either way (always the case with nits).

DarrylWong · 2023-11-07T21:47:08Z

I messed up and used the wrong logger in this sanity TC run, but confirmed the cockroach random seed print works on gceworker after switching to the test logger.

renatolabs

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @DarrylWong and @srosenberg)

Currently, roachtests must manually upload their own cockroach binary if needed through the Put API. However, almost all roachtests upload the standard t.Cockroach() binary to ./cockroach on all nodes, resulting in the same Put code being duplicated at the start of most tests. Additionally, to collect artifacts we still need a cockroach binary at a discoverable path, leading to the same binary being copied twice in many cases, see: cockroachdb#97814 This change adds a TestSpec option which lets tests specify a cockroach binary to use. If one is not specified, we now upload the t.Cockroach() binary to ./cockroach. This lets us remove cockroach-default logic for artifacts, and removes the need to manually upload binaries at the start of each test. Release note: None Fixes: cockroachdb#104729

This change prints the cockroach random seed used by metamorphic builds to test.log for ease of debugging. Before this seed was found only in cockroach.log.

Before if a test uploaded a non t.Cockroach() binary github posting would have no knowledge of that and naively use t.Cockroach() to determine metamorphism or not. This fixes that.

DarrylWong · 2023-11-08T15:29:49Z

Added a last second addition that fixes github label posting for tests that don't use t.Cockroach(). This is easy now that we have the test spec, before github posting had no knowledge of what tests decided to upload. I think it should be a very low impact change but happy to push it back to a followup PR. Otherwise I'll go ahead and ship it.

renatolabs · 2023-11-08T15:32:48Z

Looks good, thanks @DarrylWong!

DarrylWong · 2023-11-08T16:24:10Z

TFTRs!!!

bors r=renatolabs, RaduBerinde

craig · 2023-11-08T17:58:47Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-11-08T17:58:55Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 009c53d to blathers/backport-release-23.1-113301: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.

error creating merge commit from 009c53d to blathers/backport-release-23.2-113301: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.2.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

rafiss · 2023-11-15T22:04:45Z

no need to rush, but i just wanted to check if this is getting backported to 23.1? if so, i will wait on it before merging #114197

DarrylWong · 2023-11-15T22:16:23Z

Yes but the backport is dependent on #113894 being backported and the plan for that is to wait until we can shake out all the runtime assertion flakes. Given the amount of flakes we've seen so far, I don't think we're in a rush to but someone else feel free to correct me.

Maybe it's worth it to just stick a c.Put() in there for the backport if you think the additional few weeks of test coverage is worth it? I can always remove it as part of this backport and worst case it wouldn't break anything if forgotten about.

rafiss · 2023-11-16T22:09:23Z

Maybe it's worth it to just stick a c.Put() in there for the backport if you think the additional few weeks of test coverage is worth it?

sounds good! i will do that

DarrylWong mentioned this pull request Oct 31, 2023

roachtest: randomly run with runtime assertions by default #111949

Merged

DarrylWong force-pushed the refactor-put branch 7 times, most recently from 057d12b to cdd2b40 Compare November 3, 2023 15:39

DarrylWong marked this pull request as ready for review November 6, 2023 15:21

DarrylWong requested review from a team as code owners November 6, 2023 15:21

DarrylWong requested review from srosenberg and renatolabs and removed request for a team November 6, 2023 15:21

RaduBerinde approved these changes Nov 7, 2023

View reviewed changes

renatolabs approved these changes Nov 7, 2023

View reviewed changes

renatolabs mentioned this pull request Nov 7, 2023

roachtest: print global seed to test.log #113950

Closed

DarrylWong changed the title ~~Refactor put~~ roachtest: allow tests to specify a cockroach binary to use Nov 7, 2023

DarrylWong force-pushed the refactor-put branch from cdd2b40 to de4d06b Compare November 7, 2023 19:40

DarrylWong commented Nov 7, 2023

View reviewed changes

DarrylWong force-pushed the refactor-put branch from 96616bd to 2dce87f Compare November 7, 2023 20:06

renatolabs reviewed Nov 7, 2023

View reviewed changes

DarrylWong added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. labels Nov 7, 2023

DarrylWong force-pushed the refactor-put branch from 2dce87f to e236298 Compare November 7, 2023 21:33

renatolabs approved these changes Nov 8, 2023

View reviewed changes

DarrylWong added 3 commits November 8, 2023 10:13

roachtest: print cockroach random seed to test.log

de67b88

This change prints the cockroach random seed used by metamorphic builds to test.log for ease of debugging. Before this seed was found only in cockroach.log.

roachtest: use cockroach binary test spec for github issue labels

509c570

Before if a test uploaded a non t.Cockroach() binary github posting would have no knowledge of that and naively use t.Cockroach() to determine metamorphism or not. This fixes that.

DarrylWong force-pushed the refactor-put branch from e236298 to 509c570 Compare November 8, 2023 15:27

DarrylWong mentioned this pull request Nov 8, 2023

util: support specifying catalog of metamorphic constant values #114039

Open

craig bot merged commit 46220d9 into cockroachdb:master Nov 8, 2023
8 checks passed

DarrylWong mentioned this pull request Nov 8, 2023

release-23.2: roachtest: allow tests to specify a cockroach binary to use #114060

Merged

yuzefovich mentioned this pull request Nov 10, 2023

roachtest: tpch_concurrency failed #114204

Closed

renatolabs mentioned this pull request Nov 10, 2023

roachtest: acceptance/mismatched-locality failed #114218

Closed

rafiss mentioned this pull request Nov 13, 2023

release-23.1: sql: only validate new regions when adding/dropping #114197

Merged

renatolabs mentioned this pull request Nov 17, 2023

roachtest: disable assertions and metamorphic constants #114618

Merged

DarrylWong mentioned this pull request Nov 17, 2023

release-23.1: roachtest: allow tests to specify a cockroach binary to use #114654

Merged

rafiss mentioned this pull request Dec 7, 2023

roachtest: add new roachtest to run pg_regress against crdb #115187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachtest: allow tests to specify a cockroach binary to use #113301

roachtest: allow tests to specify a cockroach binary to use #113301

DarrylWong commented Oct 30, 2023 •

edited

Loading

cockroach-teamcity commented Oct 30, 2023

DarrylWong commented Oct 30, 2023 •

edited

Loading

RaduBerinde left a comment

renatolabs commented Nov 7, 2023

renatolabs left a comment

DarrylWong left a comment

RaduBerinde commented Nov 7, 2023

renatolabs left a comment

RaduBerinde commented Nov 7, 2023

DarrylWong commented Nov 7, 2023 •

edited

Loading

renatolabs left a comment

DarrylWong commented Nov 8, 2023 •

edited

Loading

renatolabs commented Nov 8, 2023

DarrylWong commented Nov 8, 2023

craig bot commented Nov 8, 2023

blathers-crl bot commented Nov 8, 2023

rafiss commented Nov 15, 2023

DarrylWong commented Nov 15, 2023

rafiss commented Nov 16, 2023

roachtest: allow tests to specify a cockroach binary to use #113301

roachtest: allow tests to specify a cockroach binary to use #113301

Conversation

DarrylWong commented Oct 30, 2023 • edited Loading

cockroach-teamcity commented Oct 30, 2023

DarrylWong commented Oct 30, 2023 • edited Loading

RaduBerinde left a comment

Choose a reason for hiding this comment

renatolabs commented Nov 7, 2023

renatolabs left a comment

Choose a reason for hiding this comment

DarrylWong left a comment

Choose a reason for hiding this comment

RaduBerinde commented Nov 7, 2023

renatolabs left a comment

Choose a reason for hiding this comment

RaduBerinde commented Nov 7, 2023

DarrylWong commented Nov 7, 2023 • edited Loading

renatolabs left a comment

Choose a reason for hiding this comment

DarrylWong commented Nov 8, 2023 • edited Loading

renatolabs commented Nov 8, 2023

DarrylWong commented Nov 8, 2023

craig bot commented Nov 8, 2023

blathers-crl bot commented Nov 8, 2023

rafiss commented Nov 15, 2023

DarrylWong commented Nov 15, 2023

rafiss commented Nov 16, 2023

DarrylWong commented Oct 30, 2023 •

edited

Loading

DarrylWong commented Oct 30, 2023 •

edited

Loading

DarrylWong commented Nov 7, 2023 •

edited

Loading

DarrylWong commented Nov 8, 2023 •

edited

Loading